Fast-path framework bookkeeping fields in state attribute access#6757
Fast-path framework bookkeeping fields in state attribute access#6757Alek99 wants to merge 3 commits into
Conversation
Merging this PR will regress 2 benchmarks
|
Greptile SummaryThis PR speeds up framework state handling and adds broader performance coverage. The main changes are:
Confidence Score: 5/5No additional blocking issue qualifies for this review round.
Important Files Changed
Reviews (2): Last reviewed commit: "Add changelog fragment" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 483d2d1253
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for substate_class in cls.get_substates(): | ||
| substate_class.vars.setdefault(name, var) | ||
| # inherited_vars may alias this class's vars dict. | ||
| substate_class._skip_var_names = None |
There was a problem hiding this comment.
When add_var() is called on a state that already has nested substates, this clears the cache only for immediate children. Grandchildren have their inherited_vars updated indirectly because it aliases the child vars dict, but their _skip_var_names frozenset was already populated during class creation and remains stale, so Grandchild.get_skip_vars() omits the newly inherited dynamic var. Please invalidate descendants as well when propagating a dynamically added var.
Useful? React with 👍 / 👎.
_get_attribute was 43% of profiled event-handling time; much of it was the framework's own reads of dirty_vars, parent_state, substates, and _backend_vars paying the full inherited-vars/event-handler/proxying chain on every access. - Add those names (and dirty_substates) to the CLASS_VAR_NAMES fast path; they are never state vars, never inherited, and never proxied. - Check base_vars/backend_vars membership before is_mutable_type when deciding whether to proxy, so non-var values skip the type check. - Cache get_skip_vars() per class (invalidated when inherited_vars changes) instead of rebuilding the set on every internal write. - Cache the frontend computed var names used by get_delta per class (invalidated when computed_vars changes). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G8cXh3TUbNtbjm2ERqE62X
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G8cXh3TUbNtbjm2ERqE62X
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G8cXh3TUbNtbjm2ERqE62X
97e9e37 to
1ded8bd
Compare
2 participants
Linear: ENG-10096
Description
_get_attributewas ~43% of profiled event-handling time, and much of that was the framework's own reads ofdirty_vars,parent_state,substates, and_backend_varspaying the full inherited-vars/event-handler/proxying chain on every access from_mark_dirty/_clean/get_delta.dirty_substates) to theCLASS_VAR_NAMESfast path. They are never state vars, never inherited, and were never proxied (not inbase_vars/backend_vars), so resolution viaobject.__getattribute__is behavior-identical; assignments already ended inobject.__setattr__with no dirty-marking.base_vars/backend_varsmembership is checked beforeis_mutable_type, sparing non-var values the type check.get_skip_vars()per class as a frozenset, invalidated wheninherited_varschanges (_update_substate_inherited_vars,add_var).get_deltaper class, invalidated whencomputed_varschanges (_evaluate, dynamic route vars).Benchmarks (GitHub Actions runner, run, 2 passes each)
cProfile (5000 bookkeeping reads + 500 event cycles):
_get_attributetottime drops 0.120s -> 0.061s; total function calls drop 256k -> 207k.Type of change
Changes To Core Features:
test_get_skip_vars_cached_per_class: per-class caching with inherited vars included and no cross-class leakage.test_frontend_computed_var_names_cached_per_class: backend computed vars excluded; cached identity.test_framework_bookkeeping_fields_not_proxied: bookkeeping containers stay raw while real state vars still get proxied.🤖 Generated with Claude Code
https://claude.ai/code/session_01G8cXh3TUbNtbjm2ERqE62X
Generated by Claude Code